-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[nrf noup] fix various issues with Network Commissioning cluster on Wi-Fi #404
[nrf noup] fix various issues with Network Commissioning cluster on Wi-Fi #404
Conversation
This commit makes sure that correct Network ID is provided to the Network Commissioning cluster from the platform's Wi-Fi driver. Signed-off-by: Łukasz Duda <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks.
mpNetworkStatusChangeCallback->OnNetworkingStatusChange(status, NullOptional, NullOptional); | ||
WiFiManager::WiFiInfo wifiInfo; | ||
|
||
if (CHIP_NO_ERROR == WiFiManager::Instance().GetWiFiInfo(wifiInfo)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fallback to NullOptional in case the GetWiFiInfo() fails for whatever reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be okay, however I'm not sure. I see that in all 15 platforms, some NetworkID is used, or OnNetworkingStatusChange
is skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetWiFiInfo
only succeeds when the connection attempt succeeded. Shouldn't we rather add a getter for mWantedNetwork
to obtain the network ID used in the last failed attempt as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But don't we support only one network anyway? If the last connection failed then we can't interact with the device anyway, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but the connection may also fail during commissioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed f2f, will be fixed later as it's not vital for now.
This commit fixes the issue where ScanComplete event cleared the attribute values for connection status. This commit should be reverted and replaced by Matter 1.3 revision where the network commissioning cluster has been rewritten. Signed-off-by: Łukasz Duda <[email protected]>
This commit handles a few issues with Wi-Fi connection or scanning: - Use wifi_status structure instead of incompatible WiFiRequestStatus - On connect error value > 2 do not report success - On scan error value > 1 do not report success - Provide value of mandatory LastConnectErrorValue attribute Signed-off-by: Łukasz Duda <[email protected]>
087b105
to
5b97b18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely better than it was before so approving but added a few suggestions for future improvements :)
mpNetworkStatusChangeCallback->OnNetworkingStatusChange(status, NullOptional, NullOptional); | ||
WiFiManager::WiFiInfo wifiInfo; | ||
|
||
if (CHIP_NO_ERROR == WiFiManager::Instance().GetWiFiInfo(wifiInfo)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetWiFiInfo
only succeeds when the connection attempt succeeded. Shouldn't we rather add a getter for mWantedNetwork
to obtain the network ID used in the last failed attempt as well?
This PR fixes few issues with Network Commissioning cluster on Wi-Fi platform.
Please review commit by commit.
[nrf noup] Fix handling of LastNetworkID in Wi-Fi driver
This commit ensures that
LastNetworkID
attribute is not null, not only afterConnectNetwork
command is issued, but also in case the connection is established after resboot or after connection is lost.[1709852472.587357][369591:369593] CHIP:TOO: LastNetworkID: 536368726F64696E676572205761766573203547
[nrf noup] Do not clear Last Network ID and Connection Error after scan
This bug is already fixed upstream for Matter 1.3. This fixes the issue where
LastNetworkID
attribute is cleared afterScanNetworks
command is completed.[nrf noup] Fix various Wi-Fi issues with error code handling
This commit handles a few issues with Wi-Fi connection or scanning:
*This is especially critical.